Conversation
WalkthroughRemoved in-repo access-token verification and its middleware/tests; routes now accept injected authlib authenticators and are wrapped with authlib middleware. Server wiring adds delegated auth module, static OIDC KeySet provider and a scope-check flag. Client Metadata types and OpenAPI schema tightened; dependencies and E2E tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as authlib.Middleware
participant A as Authenticator (injected)
participant K as oidclib.KeySet / Auth Module
participant H as API Handler
C->>M: HTTP Request (may include Authorization)
M->>A: Authenticate request (parse token / claims)
alt delegated verification required
A->>K: Verify token & scopes (JWK/OIDC)
K-->>A: verification result
end
alt token invalid or missing
M-->>C: 401 Unauthorized
else insufficient scopes
M-->>C: 403 Forbidden
else authenticated
M->>H: Forward request with identity context
H-->>C: 200 / resource response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/api/authorization/accesstoken_test.go (2)
109-110: Consider verifying the claims content for unprotected routes.The test captures the error but doesn't verify that claims is
nilfor unprotected routes. While the current test is correct, explicitly assertingrequire.Nil(t, claims)would better document the expected behavior.- _, err = verifyAccessToken(req, provider) + claims, err := verifyAccessToken(req, provider) require.NoError(t, err) + require.Nil(t, claims)
158-159: Add test coverage for claims content on successful verification.The test verifies that no error is returned but doesn't check that claims are properly populated. Consider adding assertions to verify the claims structure and content.
- _, err = verifyAccessToken(req, provider) + claims, err := verifyAccessToken(req, provider) require.NoError(t, err) + require.NotNil(t, claims) + // Optionally verify specific claims fieldspkg/api/authorization/middleware.go (2)
3-8: Reorder imports to follow Go conventions.Standard library imports should be separated from third-party imports with a blank line.
import ( "errors" - "github.com/formancehq/go-libs/v3/collectionutils" - "github.com/zitadel/oidc/v2/pkg/op" "net/http" + + "github.com/formancehq/go-libs/v3/collectionutils" + "github.com/zitadel/oidc/v2/pkg/op" )
28-31: Consider extracting scope validation into a helper function.The scope checking logic is duplicated with multiple calls to
collectionutils.Contains. Extracting this into a helper function would improve maintainability and make the authorization logic clearer.Add a helper function before the middleware:
func hasRequiredScope(scopes []string, method string) bool { hasRead := collectionutils.Contains(scopes, "auth:read") hasWrite := collectionutils.Contains(scopes, "auth:write") switch method { case http.MethodOptions, http.MethodGet, http.MethodHead, http.MethodTrace: return hasRead || hasWrite default: return hasWrite } }Then use it in the middleware:
-allowed := true -switch r.Method { -case http.MethodOptions, http.MethodGet, http.MethodHead, http.MethodTrace: - allowed = allowed && (collectionutils.Contains(claims.Scopes, "auth:read") || collectionutils.Contains(claims.Scopes, "auth:write")) -default: - allowed = allowed && collectionutils.Contains(claims.Scopes, "auth:write") -} - -if !allowed { +if !hasRequiredScope(claims.Scopes, r.Method) { http.Error(w, "forbidden", http.StatusForbidden) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/dependabot.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
pkg/api/authorization/accesstoken.go(1 hunks)pkg/api/authorization/accesstoken_test.go(5 hunks)pkg/api/authorization/middleware.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/api/authorization/accesstoken.go (1)
pkg/api/authorization/middleware.go (3)
ErrMissingAuthHeader(11-11)ErrMalformedAuthHeader(12-12)ErrVerifyAuthToken(13-13)
pkg/api/authorization/middleware.go (1)
pkg/client/internal/utils/utils.go (1)
Contains(59-66)
🔇 Additional comments (1)
pkg/api/authorization/accesstoken.go (1)
30-30: LGTM: Claims extraction is correct.The use of
op.VerifyAccessToken[*oidc.AccessTokenClaims]correctly captures the token claims for downstream authorization logic.
gfyrag
force-pushed
the
fix/add-scopes
branch
from
ee0a654 to
26acfed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/api/authorization/middleware.go (2)
34-40: The refactor suggestion from previous review remains applicable.The authorization logic uses a redundant pattern (
allowed := truefollowed byallowed = allowed && ...). The previous review's suggestion to simplify this logic is still valid and would improve readability.Apply the diff from the previous review:
-allowed := true +var allowed bool switch r.Method { case http.MethodOptions, http.MethodGet, http.MethodHead, http.MethodTrace: - allowed = allowed && (collectionutils.Contains(claims.Scopes, "auth:read") || collectionutils.Contains(claims.Scopes, "auth:write")) + allowed = collectionutils.Contains(claims.Scopes, "auth:read") || collectionutils.Contains(claims.Scopes, "auth:write") default: - allowed = allowed && collectionutils.Contains(claims.Scopes, "auth:write") + allowed = collectionutils.Contains(claims.Scopes, "auth:write") }
28-32: Critical: The nil check issue flagged in previous review is still unresolved.This critical issue was already identified in a previous review and remains unfixed. When
verifyAccessTokenreturns(nil, nil)for unprotected routes, the code proceeds to accessclaims.Scopesat line 37, causing a nil pointer dereference panic.The fix from the previous review should be applied:
claims, err := verifyAccessToken(r, o) if err != nil { http.Error(w, err.Error(), http.StatusUnauthorized) return } +// Skip authorization for unprotected routes +if claims == nil { + h.ServeHTTP(w, r) + return +} + allowed := true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/dependabot.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
pkg/api/authorization/accesstoken.go(1 hunks)pkg/api/authorization/accesstoken_test.go(4 hunks)pkg/api/authorization/middleware.go(2 hunks)test/e2e/client_credentials_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/api/authorization/accesstoken_test.go
- pkg/api/authorization/accesstoken.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/api/authorization/middleware.go (1)
pkg/client/internal/utils/utils.go (1)
Contains(59-66)
🔇 Additional comments (4)
pkg/api/authorization/middleware.go (2)
3-9: LGTM!The new imports appropriately support the scope-checking functionality (collectionutils for Contains, strings for HasPrefix).
42-45: LGTM!The forbidden response logic correctly returns a 403 status when scope authorization fails.
test/e2e/client_credentials_test.go (2)
46-46: LGTM!Adding the
auth:writescope to the static client configuration correctly aligns with the new authorization requirements in the middleware.
60-60: LGTM!Adding the
auth:writescope to the client credentials configuration ensures the test obtains tokens with the necessary permissions for write operations, aligning with the middleware's authorization checks.
gfyrag
force-pushed
the
fix/add-scopes
branch
from
26acfed to
faa1543
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/module.go (1)
58-64: Fix the unusedop.OpenIDProviderdependencyWith CreateRootRouter no longer using the provider, the injected parameter
o op.OpenIDProvideris now unused, which will failgo buildbecause Go rejects unused variables. Either rename it to_or drop the dependency entirely.- fx.Invoke(func(lc fx.Lifecycle, r chi.Router, healthController *health.HealthController, o op.OpenIDProvider) { + fx.Invoke(func(lc fx.Lifecycle, r chi.Router, healthController *health.HealthController, _ op.OpenIDProvider) {
🧹 Nitpick comments (1)
pkg/api/users_test.go (1)
90-90: Consider adding tests for authentication and scope validation.While using
authlib.NewNoAuth()is appropriate for unit tests focused on business logic, the PR objective is to add scope checks. Consider adding integration tests that:
- Verify that unauthenticated requests to
/usersendpoints are rejected- Verify that authenticated requests without required scopes are rejected
- Verify that authenticated requests with proper scopes succeed
This would ensure the authentication middleware and scope checks are working as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
.github/dependabot.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (12)
cmd/serve.go(4 hunks)pkg/api/authorization/accesstoken.go(0 hunks)pkg/api/authorization/accesstoken_test.go(0 hunks)pkg/api/authorization/main_test.go(0 hunks)pkg/api/authorization/middleware.go(0 hunks)pkg/api/clients.go(2 hunks)pkg/api/clients_test.go(2 hunks)pkg/api/module.go(2 hunks)pkg/api/users.go(2 hunks)pkg/api/users_test.go(2 hunks)pkg/oidc/storage.go(1 hunks)test/e2e/client_credentials_test.go(2 hunks)
💤 Files with no reviewable changes (4)
- pkg/api/authorization/accesstoken.go
- pkg/api/authorization/accesstoken_test.go
- pkg/api/authorization/main_test.go
- pkg/api/authorization/middleware.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/client_credentials_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/api/module.go (1)
pkg/testserver/server.go (1)
Logger(63-65)
cmd/serve.go (5)
pkg/api/module.go (1)
Module(46-66)pkg/storage/sqlstorage/module.go (1)
Module(19-40)pkg/oidc/module.go (1)
Module(19-43)pkg/delegatedauth/module.go (1)
Module(10-17)pkg/oidc/storage.go (2)
Service(89-91)KeyID(551-551)
🔇 Additional comments (3)
pkg/api/users.go (2)
4-4: LGTM: Import added for authentication support.The import addition is necessary for the authentication middleware integration.
14-21: Add documentation for required scopes on user endpoints.The
/usersendpoints are wrapped withauthlib.Middleware, but scope enforcement cannot be verified from this codebase since the middleware is implemented in an external package (github.com/formancehq/go-libs/v3/auth).Scopes are defined in the generated client code (pkg/client/v1.go): both
listUsersandreadUserrequire"auth:read". However, the handler functions lack documentation about these scope requirements, making the intent unclear.Add comments above the route handlers documenting the required scopes:
// Requires "auth:read" scope r.Get("/", listUsers(db))This improves code maintainability and clarifies security requirements, especially if scope validation is delegated to the middleware layer.
pkg/api/users_test.go (1)
5-5: LGTM: Import added for test authenticator.The import is necessary to provide the
NewNoAuth()authenticator for test setup.
gfyrag
force-pushed
the
fix/add-scopes
branch
2 times, most recently
from
9ec43ed to
5d44230
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
go.sumis excluded by!**/*.sumpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!pkg/client/**pkg/client/.speakeasy/gen.yamlis excluded by!pkg/client/**pkg/client/docs/models/components/client.mdis excluded by!pkg/client/**pkg/client/docs/models/components/clientsecret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createclientrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createsecretrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/secret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/updateclientrequest.mdis excluded by!pkg/client/**pkg/client/formance.gois excluded by!pkg/client/**pkg/client/models/components/client.gois excluded by!pkg/client/**pkg/client/models/components/clientsecret.gois excluded by!pkg/client/**pkg/client/models/components/createclientrequest.gois excluded by!pkg/client/**pkg/client/models/components/createsecretrequest.gois excluded by!pkg/client/**pkg/client/models/components/secret.gois excluded by!pkg/client/**pkg/client/models/components/updateclientrequest.gois excluded by!pkg/client/**
📒 Files selected for processing (18)
.coderabbit.yaml(1 hunks).github/dependabot.yml(0 hunks)cmd/serve.go(4 hunks)go.mod(6 hunks)openapi.yaml(1 hunks)pkg/api/authorization/accesstoken.go(0 hunks)pkg/api/authorization/accesstoken_test.go(0 hunks)pkg/api/authorization/main_test.go(0 hunks)pkg/api/authorization/middleware.go(0 hunks)pkg/api/clients.go(3 hunks)pkg/api/clients_test.go(2 hunks)pkg/api/module.go(2 hunks)pkg/api/users.go(2 hunks)pkg/api/users_test.go(2 hunks)pkg/oidc/storage.go(1 hunks)pkg/testserver/server.go(3 hunks)test/e2e/client_credentials_test.go(2 hunks)test/e2e/oauth2_clients_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- pkg/api/authorization/accesstoken.go
- pkg/api/authorization/middleware.go
- pkg/api/authorization/accesstoken_test.go
- .github/dependabot.yml
- pkg/api/authorization/main_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/testserver/server.go
- pkg/api/users.go
- pkg/oidc/storage.go
- pkg/api/clients.go
- test/e2e/client_credentials_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/oauth2_clients_test.go (10)
pkg/testserver/server.go (4)
Server(69-77)New(279-299)Configuration(54-63)DelegatedConfiguration(42-46)pkg/client/models/components/clientsecret.go (1)
ClientSecret(5-10)pkg/client.go (2)
StaticClient(118-121)ClientOptions(154-164)pkg/client/models/components/client.go (1)
Client(5-16)pkg/client/models/components/createclientrequest.go (1)
CreateClientRequest(5-14)pkg/client/models/operations/readclient.go (1)
ReadClientRequest(9-12)pkg/client/models/components/updateclientrequest.go (1)
UpdateClientRequest(5-14)pkg/client/models/operations/deleteclient.go (2)
DeleteClientResponse(21-23)DeleteClientRequest(9-12)pkg/client/models/components/createsecretrequest.go (1)
CreateSecretRequest(5-8)pkg/client/models/operations/deletesecret.go (1)
DeleteSecretRequest(9-14)
cmd/serve.go (5)
pkg/api/module.go (1)
Module(46-66)pkg/storage/sqlstorage/module.go (1)
Module(19-40)pkg/oidc/module.go (1)
Module(19-43)pkg/delegatedauth/module.go (1)
Module(10-17)pkg/oidc/storage.go (2)
Service(89-91)KeyID(551-551)
🔇 Additional comments (9)
.coderabbit.yaml (1)
1-3: Verify intent: Excludingpkg/client/**from review coverage.The configuration excludes the entire
pkg/clientdirectory from CodeRabbit reviews. According to the PR summary, this directory contains "extensive changes to client-related data models." Please confirm this exclusion is intentional, especially if these client changes represent material modifications that should undergo review.If the exclusion is necessary (e.g., auto-generated or machine-formatted code), consider adding a clarifying comment or narrowing the pattern to exclude only generated files.
go.mod (3)
96-98: Verify new transitive dependencies are necessary and properly constrained.The following new indirect dependencies were introduced:
github.com/hashicorp/go-cleanhttp v0.5.2andgithub.com/hashicorp/go-retryablehttp v0.7.8(HTTP retry/cleanup utilities)github.com/zitadel/oidc/v3 v3.45.0andgithub.com/zitadel/schema v1.3.1(OIDC schema support)These appear to be pulled in transitively (possibly through
go-libs/v3update on line 11). Confirm that:
- These dependencies are intentional and required for scope checking.
- The
go-libs/v3version bump (line 11) is the source of these transitive deps.- No version conflicts arise from these additions.
Also applies to: 145-146
26-26: Confirm OTEL version coherence is intentional; minor-version backward compatibility supports the current configuration.OpenTelemetry SDK packages must remain backwards compatible, and the v1.37.0 to v1.38.0 bump is a semantic minor-version update. The verified mismatch—core/trace at v1.38.0 and exporters at v1.37.0—should not introduce runtime incompatibilities. However, ensure this versioning strategy is deliberate and aligns with your dependency management approach.
11-29: The review comment is incorrect—these dependency patterns are standard and verified.Go's module system handles multiple major versions correctly when import paths differ. The
go mod verifycheck passes, confirming no actual conflicts exist. The coexistence of:
zitadel/oidc/v2@v2.12.2(direct) andzitadel/oidc/v3@v3.45.0(direct)go-jose/v4alongsidegopkg.in/go-jose/go-jose.v2andgopkg.in/square/go-jose.v2...is expected behavior—each represents a separate, importable package. Additionally,
golang.org/x/oauth2 v0.31.0introduces no breaking changes; it only updates the Go toolchain requirement to 1.24.0.The changes are valid and impose no compatibility risks.
Likely an incorrect or invalid review comment.
pkg/api/users_test.go (1)
90-90: Authenticator injection keeps unit tests aligned
authlib.NewNoAuth()satisfies the new route signature without introducing auth side effects, so the existing handler assertions still run unchanged.pkg/api/clients_test.go (1)
44-44: No-op authenticator keeps client tests focusedRouting now accepts the authenticator, and passing
authlib.NewNoAuth()lets the suite exercise the handlers without layering authentication concerns.cmd/serve.go (1)
202-208: Static KeySet ensures local token verificationDecorating the
oidclib.KeySetwith the server’s own RSA public key means authlib doesn’t reach out to remote JWKS and the new scope checks operate against a deterministic signing source.pkg/api/module.go (1)
18-39: Issuer propagation preserved after auth refactorPopulating
op.ContextWithIssuerin the router middleware keeps downstream handlers aware of the issuer even after removing the old authorization layer, so existing OIDC-dependent code continues to work.test/e2e/oauth2_clients_test.go (1)
620-739: End-to-end scope coverage looks solidExercising the client flows again with an access token lacking
auth:writegives us confidence the new scope checks reject every CRUD path, matching the production expectations.
gfyrag
force-pushed
the
fix/add-scopes
branch
2 times, most recently
from
0af8f5d to
6307dad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/api/module.go (1)
58-58: Consider removing unusedOpenIDProviderparameter.The
o op.OpenIDProviderparameter appears unused in the function body. If it's no longer needed after the auth refactor, remove it for clarity.If it's not needed, apply this diff:
- fx.Invoke(func(lc fx.Lifecycle, r chi.Router, healthController *health.HealthController, o op.OpenIDProvider) { + fx.Invoke(func(lc fx.Lifecycle, r chi.Router, healthController *health.HealthController) {Alternatively, run this script to check if
OpenIDProviderinitialization has necessary side-effects elsewhere:#!/bin/bash # Description: Check if OpenIDProvider is constructed/used elsewhere in the module for side-effects # Find OpenIDProvider construction or configuration rg -n 'op\.OpenIDProvider|NewOpenIDProvider' --type go -C 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
go.sumis excluded by!**/*.sumpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!pkg/client/**pkg/client/.speakeasy/gen.yamlis excluded by!pkg/client/**pkg/client/docs/models/components/client.mdis excluded by!pkg/client/**pkg/client/docs/models/components/clientsecret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createclientrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createsecretrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/secret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/updateclientrequest.mdis excluded by!pkg/client/**pkg/client/formance.gois excluded by!pkg/client/**pkg/client/models/components/client.gois excluded by!pkg/client/**pkg/client/models/components/clientsecret.gois excluded by!pkg/client/**pkg/client/models/components/createclientrequest.gois excluded by!pkg/client/**pkg/client/models/components/createsecretrequest.gois excluded by!pkg/client/**pkg/client/models/components/secret.gois excluded by!pkg/client/**pkg/client/models/components/updateclientrequest.gois excluded by!pkg/client/**
📒 Files selected for processing (18)
.coderabbit.yaml(1 hunks).github/dependabot.yml(0 hunks)cmd/serve.go(4 hunks)go.mod(6 hunks)openapi.yaml(1 hunks)pkg/api/authorization/accesstoken.go(0 hunks)pkg/api/authorization/accesstoken_test.go(0 hunks)pkg/api/authorization/main_test.go(0 hunks)pkg/api/authorization/middleware.go(0 hunks)pkg/api/clients.go(3 hunks)pkg/api/clients_test.go(2 hunks)pkg/api/module.go(2 hunks)pkg/api/users.go(2 hunks)pkg/api/users_test.go(2 hunks)pkg/oidc/storage.go(1 hunks)pkg/testserver/server.go(3 hunks)test/e2e/client_credentials_test.go(2 hunks)test/e2e/oauth2_clients_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- pkg/api/authorization/middleware.go
- pkg/api/authorization/main_test.go
- pkg/api/authorization/accesstoken.go
- pkg/api/authorization/accesstoken_test.go
- .github/dependabot.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- openapi.yaml
- pkg/oidc/storage.go
- test/e2e/client_credentials_test.go
- pkg/api/clients_test.go
- pkg/api/clients.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/serve.go (4)
pkg/api/module.go (1)
Module(46-66)pkg/storage/sqlstorage/module.go (1)
Module(19-40)pkg/oidc/module.go (1)
Module(19-43)pkg/oidc/storage.go (2)
Service(89-91)KeyID(551-551)
test/e2e/oauth2_clients_test.go (11)
pkg/testserver/server.go (4)
Server(69-77)New(279-299)Configuration(54-63)DelegatedConfiguration(42-46)pkg/client/formance.go (1)
New(141-171)pkg/client/models/components/clientsecret.go (1)
ClientSecret(5-10)pkg/client.go (2)
StaticClient(118-121)ClientOptions(154-164)pkg/client/models/components/client.go (1)
Client(5-16)pkg/client/models/components/createclientrequest.go (1)
CreateClientRequest(5-14)pkg/client/models/operations/readclient.go (1)
ReadClientRequest(9-12)pkg/client/models/components/updateclientrequest.go (1)
UpdateClientRequest(5-14)pkg/client/models/operations/deleteclient.go (1)
DeleteClientRequest(9-12)pkg/client/models/components/createsecretrequest.go (1)
CreateSecretRequest(5-8)pkg/client/models/operations/deletesecret.go (1)
DeleteSecretRequest(9-14)
🔇 Additional comments (27)
.coderabbit.yaml (1)
1-3: ****The exclusion of
pkg/client/**from CodeRabbit reviews is intentional and correct. Thepkg/clientdirectory contains a Speakeasy-generated OpenAPI SDK, not manually maintained source code. The repository's CONTRIBUTING.md explicitly states this directory contains generated code and does not accept direct changes. The metadata type changes observed in this PR (frommap[string]*any*tomap[string]*string*) are automatically regenerated stubs from an updated OpenAPI specification version (0.7.2), not hand-written code requiring review. Excluding auto-generated artifacts from CodeRabbit follows best practices and the project's established policy.Likely an incorrect or invalid review comment.
pkg/testserver/server.go (3)
7-7: LGTM! Clean import alias.The import alias
authlibis clear and prevents naming conflicts with the localauthpackage.
62-62: LGTM! Configuration field added.The
CheckScopesfield appropriately extends the test server configuration to support scope checking in tests.
190-192: LGTM! Scope check flag properly wired.The conditional logic correctly appends the scope check flag when enabled in the configuration.
pkg/api/users_test.go (2)
5-5: LGTM! Import added for authentication support.
90-90: LGTM! Test updated to match new API signature.The use of
authlib.NewNoAuth()is appropriate for unit tests that don't need authentication enforcement.pkg/api/users.go (2)
4-4: LGTM! Import added for authentication support.
14-21: LGTM! Routes properly protected with authentication middleware and all callers have been updated.The function signature change to include an
authenticatorparameter and the use ofauthlib.Middleware(authenticator)correctly implements the scope checking functionality. Verification confirms the only caller inpkg/api/users_test.go:90has been updated with the new 3-parameter signature.cmd/serve.go (6)
8-10: LGTM! Imports added for authentication and OIDC support.
82-99: LGTM! HTTP client extraction improves reusability.Extracting
otlpHttpClientas a standalone function is a good refactor that makes the HTTP client configuration reusable.
114-114: LGTM! Scope checking flag added.The
AuthCheckScopesFlagproperly enables scope checking functionality as per the PR objectives.
180-180: LGTM! Flag value properly read.
186-194: Reformatting is consistent.The multi-line formatting of
api.Modulearguments improves readability.
195-208: LGTM! Authentication module properly configured.The
authlib.Moduleconfiguration correctly wires up the authentication with:
- Enabled flag
- Issuer set to baseUrl
- CheckScopes flag from CLI
- Service name
The
oidclib.KeySetprovider using the RSA public key from the signing key is properly implemented. The KeyID referencesoidc.KeyIDwhich should be consistent with the key identifier used elsewhere in the OIDC implementation.test/e2e/oauth2_clients_test.go (9)
1-21: LGTM! Clean test setup with proper imports.The build tag
//go:build itcorrectly isolates integration tests, and the imports are well-organized.
22-52: LGTM! Well-structured test server configuration.The test setup properly configures:
- PostgreSQL database connection
- Delegated OIDC configuration with mockOIDC
- A static client with
auth:writescope for testingCheckScopes: trueto enable scope verificationThis provides a realistic test environment for validating the scope checking functionality.
54-140: LGTM! Comprehensive client creation tests.The tests validate:
- Minimal client creation (name only)
- Full client creation with all fields
- Proper field persistence and defaults
Good coverage of the create endpoint.
142-200: LGTM! Thorough client read tests.Tests cover both successful reads of existing clients and error handling for non-existent clients.
202-275: LGTM! Comprehensive client listing tests.The tests validate:
- Successful listing of multiple clients
- Client details are properly returned
- Filtering/inclusion of created clients
277-369: LGTM! Complete client update tests.Tests cover:
- Successful updates with all fields
- Persistence verification by re-reading the client
- Error handling for non-existent clients
371-429: LGTM! Client deletion tests with idempotency check.The tests properly validate:
- Successful deletion of existing clients
- Verification that the client is removed
- Idempotent behavior for deleting non-existent clients
The use of
BeElementOf(http.StatusNoContent, http.StatusNotFound)on line 426 correctly handles both possible idempotent responses.
431-572: LGTM! Comprehensive secret management tests.Excellent coverage of:
- Creating secrets with metadata
- Validating clear text is returned only on creation
- Multiple secrets per client
- Secret deletion
- Error cases for non-existent clients/secrets
The test on lines 476-487 properly validates that secrets are added to the client.
575-741: LGTM! Excellent authorization tests.The unauthorized client tests thoroughly validate that operations are properly denied when the client lacks the
auth:writescope:
- Create client
- List clients
- Read client
- Update client
- Delete client
- Create secret
- Delete secret
This provides strong validation of the scope checking functionality introduced in this PR.
go.mod (2)
11-11: No action required. The go-libs/v3 version is valid and accessible. The authlib/auth module is actively imported and used across 6 files in the codebase (cmd/serve.go, pkg/testserver/server.go, pkg/api/users.go, pkg/api/clients.go, and their test files), confirming the pre-release version intentionally includes the required functionality.
13-13: No issues found—go-jose v4.1.3 is the latest version and contains all security patches.Verification confirms that go-jose/v4 v4.1.3 is the latest available version (released October 2025) and is not affected by any known security vulnerabilities. The two identified advisories (DoS and Data Amplification) are patched in v4.0.5 and v4.0.1 respectively, both of which are older than the current version. The dependency is secure.
pkg/api/module.go (2)
50-52: LGTM!The fx.Provide wiring correctly updated to match the new
CreateRootRoutersignature, properly injecting the logger while capturing issuer and debug from the module scope.
18-22: Now I need to examine the actual file to verify the line 58 concern and see the complete Module function:Based on the executed
rgscript output, I can now verify the original review comment:Verification Summary:
The search found the CreateRootRouter function and its usage:
- Line 18: Function definition with new signature
(logger, issuer, debug)- removes OpenIDProvider ✓- Line 51: Single call site in fx.Provide wrapper calling
CreateRootRouter(logger, issuer, debug)- matches new signature ✓The original review's main concern has been verified as correct: all callers of CreateRootRouter have been updated to match the new signature. There is only one call site in the codebase (at line 51), and it has been properly updated with the 3-parameter call matching the new function signature.
LGTM: All callers of CreateRootRouter have been correctly updated to match the new signature.
The signature change successfully removes the OpenIDProvider parameter, aligning with the PR's goal to shift away from local token verification toward delegated authentication. The router middleware chain continues to propagate the issuer via context as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openapi.yaml (1)
256-257: Breaking API change already flagged in previous review—verify resolution completeness.This Metadata.additionalProperties change (from unrestricted to
type: string) was comprehensively analyzed in a previous review and flagged as a critical breaking change. That review indicated it was addressed in commit 0af8f5d.To confirm the resolution is complete, verify that:
- CHANGELOG or release notes document this breaking change
- Migration guidance is provided for consumers passing non-string metadata values
- This change is clearly communicated in the release notes for the affected version
If documentation updates were included in commit 0af8f5d, feel free to disregard this note. Otherwise, ensure consumer-facing guidance is added before release.
🧹 Nitpick comments (2)
pkg/api/users_test.go (1)
5-5: LGTM! Consider adding authentication-aware tests.The changes correctly adapt to the new
addUserRoutessignature. Usingauthlib.NewNoAuth()is appropriate for testing core CRUD functionality without authentication complexity.However, since this PR adds scope checks, consider adding complementary tests that:
- Verify authentication is properly enforced on user routes
- Validate scope-based authorization (e.g., ensure certain operations require specific scopes)
- Test negative cases where requests without proper authentication/scopes are rejected
Also applies to: 90-90
test/e2e/oauth2_clients_test.go (1)
722-739: Consider enhancing the delete secret authorization test.The test uses a dummy secret ID (line 732) because the unauthorized client cannot read the client to obtain a real secret ID. While this still validates that unauthorized requests are rejected, it's ambiguous whether the failure is due to:
- Authorization failure (intended test)
- Non-existent secret (side effect)
Consider one of these approaches for clearer intent:
- Store the real secret ID during the setup (lines 602-609) in a variable accessible to this test
- Add a comment clarifying that either failure mode (auth or not-found) is acceptable for security
- If the API fails-fast on authorization before resource lookup, document that guarantee
Example enhancement:
When("trying to delete a secret", func() { + var realSecretID string var ( err error ) BeforeEach(func() { - // First, we need to get the secret ID from the client - // But we can't read the client, so we'll use a dummy ID + // Note: Using a real secret ID from setup to test authorization, + // not resource existence. The API should reject based on scope + // before checking if the resource exists. + realSecretID = // capture from lines 602-609 setup _, err = srv.Client(unauthorizedHTTPClient).Auth.V1.DeleteSecret(ctx, operations.DeleteSecretRequest{ ClientID: unauthorizedClientID, - SecretID: "dummy-secret-id", + SecretID: realSecretID, }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
go.sumis excluded by!**/*.sumpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!pkg/client/**pkg/client/.speakeasy/gen.yamlis excluded by!pkg/client/**pkg/client/docs/models/components/client.mdis excluded by!pkg/client/**pkg/client/docs/models/components/clientsecret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createclientrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createsecretrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/secret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/updateclientrequest.mdis excluded by!pkg/client/**pkg/client/formance.gois excluded by!pkg/client/**pkg/client/models/components/client.gois excluded by!pkg/client/**pkg/client/models/components/clientsecret.gois excluded by!pkg/client/**pkg/client/models/components/createclientrequest.gois excluded by!pkg/client/**pkg/client/models/components/createsecretrequest.gois excluded by!pkg/client/**pkg/client/models/components/secret.gois excluded by!pkg/client/**pkg/client/models/components/updateclientrequest.gois excluded by!pkg/client/**
📒 Files selected for processing (18)
.coderabbit.yaml(1 hunks).github/dependabot.yml(0 hunks)cmd/serve.go(4 hunks)go.mod(3 hunks)openapi.yaml(1 hunks)pkg/api/authorization/accesstoken.go(0 hunks)pkg/api/authorization/accesstoken_test.go(0 hunks)pkg/api/authorization/main_test.go(0 hunks)pkg/api/authorization/middleware.go(0 hunks)pkg/api/clients.go(3 hunks)pkg/api/clients_test.go(2 hunks)pkg/api/module.go(2 hunks)pkg/api/users.go(2 hunks)pkg/api/users_test.go(2 hunks)pkg/oidc/storage.go(1 hunks)pkg/testserver/server.go(3 hunks)test/e2e/client_credentials_test.go(2 hunks)test/e2e/oauth2_clients_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- pkg/api/authorization/accesstoken_test.go
- pkg/api/authorization/accesstoken.go
- pkg/api/authorization/middleware.go
- .github/dependabot.yml
- pkg/api/authorization/main_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- test/e2e/client_credentials_test.go
- go.mod
- pkg/api/module.go
- pkg/testserver/server.go
- pkg/api/clients.go
- pkg/api/clients_test.go
- pkg/api/users.go
- pkg/oidc/storage.go
- .coderabbit.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/serve.go (4)
pkg/api/module.go (1)
Module(46-66)pkg/storage/sqlstorage/module.go (1)
Module(19-40)pkg/oidc/module.go (1)
Module(19-43)pkg/oidc/storage.go (2)
Service(89-91)KeyID(551-551)
test/e2e/oauth2_clients_test.go (10)
pkg/testserver/server.go (4)
Server(69-77)New(279-299)Configuration(54-63)DelegatedConfiguration(42-46)pkg/client/models/components/clientsecret.go (1)
ClientSecret(5-10)pkg/client.go (2)
StaticClient(118-121)ClientOptions(154-164)pkg/client/models/components/client.go (1)
Client(5-16)pkg/client/models/components/createclientrequest.go (1)
CreateClientRequest(5-14)pkg/client/models/operations/readclient.go (1)
ReadClientRequest(9-12)pkg/client/models/components/updateclientrequest.go (1)
UpdateClientRequest(5-14)pkg/client/models/operations/deleteclient.go (2)
DeleteClientResponse(21-23)DeleteClientRequest(9-12)pkg/client/models/components/createsecretrequest.go (1)
CreateSecretRequest(5-8)pkg/client/models/operations/deletesecret.go (1)
DeleteSecretRequest(9-14)
🔇 Additional comments (12)
test/e2e/oauth2_clients_test.go (7)
1-20: LGTM! Clean test setup.The build tag, package declaration, and imports are appropriate for an E2E test suite. The use of Ginkgo/Gomega framework and OAuth2 client credentials flow aligns well with testing OAuth2 client management endpoints.
22-52: LGTM! Comprehensive test server configuration.The test setup properly configures:
- PostgreSQL backend for persistence
- OIDC delegated authentication
- A static client with
auth:writescope for authorized operationsCheckScopes: trueto enable scope validation testingThis provides a solid foundation for testing both authorized and unauthorized access patterns.
69-369: LGTM! Comprehensive CRUD test coverage.The client CRUD tests are well-structured and thorough:
- Proper separation of minimal vs. full field creation scenarios
- Validation of all returned fields
- Edge case handling (non-existent resources)
- Persistence verification after updates (lines 340-351)
The use of nested
Whenblocks makes the test intent clear and follows BDD best practices.
410-427: Verify idempotent delete behavior is intentional.The test accepts both
204 No Contentand404 Not Foundfor deleting a non-existent client. While the comment indicates this is expected idempotent behavior, accepting two different status codes could mask inconsistent API implementation.Consider verifying:
- What is the intended API contract for deleting non-existent resources?
- Should the behavior be deterministic (always return the same status code)?
If the dual behavior is intentional, consider documenting why both status codes are acceptable in this context.
431-572: LGTM! Thorough secret management test coverage.The secret management tests are comprehensive and well-designed:
- Verifies the complete secret structure including clear text and last digits (lines 467-474)
- Tests cumulative state with multiple secrets (lines 489-515)
- Properly validates that secrets are reflected in the parent client (lines 476-487)
- Good coverage of error scenarios (non-existent secret/client)
The clear text secret is correctly verified only in the creation response, not in subsequent reads, which aligns with security best practices.
575-619: LGTM! Excellent unauthorized access test setup.The unauthorized client setup is well-designed:
- Uses the authorized global client to create the test client
- Deliberately omits
auth:writescope (line 595)- Creates a realistic scenario where a client exists but lacks required permissions
This setup enables comprehensive authorization testing across all endpoints.
621-720: LGTM! Comprehensive authorization coverage.The unauthorized access tests provide excellent security coverage by validating that all client and secret operations are properly protected:
- CRUD operations for clients (create, list, read, update, delete)
- Secret operations (create, delete)
All tests correctly expect errors when operations are attempted without the
auth:writescope. This is critical for validating the scope-check functionality introduced in this PR.cmd/serve.go (5)
82-99: LGTM - Good refactoring for reusability.The extraction of
otlpHttpClientinto a separate helper function improves code organization and reusability. The switch fromfx.Providetofx.Decorateappropriately decorates the HTTP client in the dependency graph.
114-114: LGTM - Safe opt-in default.The scope checking flag defaults to
false, ensuring backward compatibility. This allows existing deployments to continue without scope enforcement until explicitly enabled.
8-10: Import statements are correct and all exported symbols are properly used.The verification confirms:
- Dependencies are present in go.mod with appropriate versions
authlib.AuthCheckScopesFlag,authlib.Module, andauthlib.ModuleConfigare all properly exported and usedoidclib.KeySet,oidclib.NewStaticKeySet, andoidclib.KeyUseSignatureare all properly exported and usedgo-jose/v4is properly imported and used forjose.JSONWebKey
195-200: Verify thatauthlib.ModuleConfigingithub.com/formancehq/go-libs/v3v3.3.1 supports all four fields used:Enabled,Issuer,CheckScopes, andService.The external dependency structure cannot be verified within the codebase. Confirm the struct definition in formancehq/go-libs matches the configuration passed at cmd/serve.go lines 195-200.
201-208: LGTM - KeySet configuration is correct.The static
KeySetdecoration properly provides the server's public key for JWT verification:
- Using the server's own public key is appropriate for verifying self-issued tokens
KeyID: oidc.KeyIDmatches the constant defined in the codebase (value: "id")Algorithm: RS256andUse: oidclib.KeyUseSignatureare standard for JWT verification- All constants (
oidclib.KeyUseSignature,oidclib.NewStaticKeySet,oidclib.KeySet) are valid exports fromgithub.com/formancehq/go-libs/v3/oidc
gfyrag
force-pushed
the
fix/add-scopes
branch
from
6307dad to
1a17c01
Compare
gfyrag
force-pushed
the
fix/add-scopes
branch
from
1a17c01 to
d3d0c5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/module.go (1)
58-58: Remove the unusedop.OpenIDProviderparameter from the fx.Invoke function.The variable
ois declared in the function signature but never referenced in the function body (lines 59-63). It should be removed since it provides no value and indicates incomplete refactoring after removing it fromCreateRootRouter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
go.sumis excluded by!**/*.sumpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!pkg/client/**pkg/client/.speakeasy/gen.yamlis excluded by!pkg/client/**pkg/client/docs/models/components/client.mdis excluded by!pkg/client/**pkg/client/docs/models/components/clientsecret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createclientrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/createsecretrequest.mdis excluded by!pkg/client/**pkg/client/docs/models/components/secret.mdis excluded by!pkg/client/**pkg/client/docs/models/components/updateclientrequest.mdis excluded by!pkg/client/**pkg/client/formance.gois excluded by!pkg/client/**pkg/client/models/components/client.gois excluded by!pkg/client/**pkg/client/models/components/clientsecret.gois excluded by!pkg/client/**pkg/client/models/components/createclientrequest.gois excluded by!pkg/client/**pkg/client/models/components/createsecretrequest.gois excluded by!pkg/client/**pkg/client/models/components/secret.gois excluded by!pkg/client/**pkg/client/models/components/updateclientrequest.gois excluded by!pkg/client/**
📒 Files selected for processing (18)
.coderabbit.yaml(1 hunks).github/dependabot.yml(0 hunks)cmd/serve.go(4 hunks)go.mod(4 hunks)openapi.yaml(1 hunks)pkg/api/authorization/accesstoken.go(0 hunks)pkg/api/authorization/accesstoken_test.go(0 hunks)pkg/api/authorization/main_test.go(0 hunks)pkg/api/authorization/middleware.go(0 hunks)pkg/api/clients.go(3 hunks)pkg/api/clients_test.go(2 hunks)pkg/api/module.go(2 hunks)pkg/api/users.go(2 hunks)pkg/api/users_test.go(2 hunks)pkg/oidc/storage.go(1 hunks)pkg/testserver/server.go(3 hunks)test/e2e/client_credentials_test.go(2 hunks)test/e2e/oauth2_clients_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- pkg/api/authorization/accesstoken.go
- pkg/api/authorization/accesstoken_test.go
- pkg/api/authorization/middleware.go
- pkg/api/authorization/main_test.go
- .github/dependabot.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/oauth2_clients_test.go
- pkg/api/clients.go
- pkg/api/users_test.go
- pkg/testserver/server.go
- pkg/oidc/storage.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/serve.go (2)
pkg/oidc/module.go (1)
Module(19-43)pkg/oidc/storage.go (2)
Service(89-91)KeyID(551-551)
🔇 Additional comments (7)
pkg/api/clients_test.go (2)
5-5: LGTM! Clean import alias to avoid package name conflict.The
authlibalias cleanly distinguishes the authentication library from the localauthpackage imported on line 19.
44-44: LGTM! Appropriate use of NoAuth for unit tests.Passing
authlib.NewNoAuth()is the correct approach for unit tests that focus on API logic without requiring actual authentication.pkg/api/users.go (2)
4-4: LGTM! Clean import alias.The
authlibalias is consistent with the pattern used inclients_test.goand avoids conflicts with the localauthpackage.
14-15: LGTM! Clean dependency injection pattern.The authenticator is properly injected as a parameter and applied as middleware to the entire
/usersroute group. This aligns with the PR's objective of adding scope checks via authlib.pkg/api/module.go (3)
18-22: LGTM! Cleaner router initialization.Removing the
OpenIDProviderparameter fromCreateRootRoutersimplifies the initialization. The issuer is still properly propagated viaop.ContextWithIssueron line 35.
50-51: LGTM! Simplified provider function.The provider function now correctly depends only on the logger, with
issueranddebugcaptured from the enclosing scope.
55-56: The authenticator appears to be properly provided through theauthlib.Moduledependency.Based on the codebase analysis:
authlib.Module()is supplied to the fx dependency graph incmd/serve.goat line 195- Both
addClientRoutesandaddUserRoutesrequireauthlib.Authenticatoras a parameter- These functions are invoked via
fx.Invoke()inpkg/api/module.goThe fx dependency resolver would fail at runtime if
authlib.Authenticatorwere not available throughauthlib.Module. Sinceauthlib.Moduleis from the external librarygithub.com/formancehq/go-libs/v3/auth, I cannot definitively verify its exports through public documentation. However, the code structure suggests the dependency is correctly satisfied.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores